-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
extendObservable: remove constraint for existing property #2252
extendObservable: remove constraint for existing property #2252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I think we are pretty close, left 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved! Thanks a lot.
This change might need backporting to MobX 4? cc @kr_anand_mishra
Yeah, I actually specifically need it for Mobx4 :) |
Do you have an idea how long until it will be released, by the way? |
I'm a little behind with cutting releases, but early next week should be doable I think. Would that work for you? If interested, feel free to open a PR against mobx4-master as well and cherry-pick the changes (only test and implementation) We are working on making that process smoother😅 |
Yeah, early next week should be fine. I’ll make an MR for mobx4-master first thing in the morning with just the code and tests. |
THanks! |
Currently extendObservable throws an error if you try to use it on a property which already exists in the target object/class. This makes it impossible to dynamically make an existing property observable.
/docs
. For new functionality, at leastAPI.md
should be updatednpm run perf
)Feel free to ask help with any of these boxes!
The above process doesn't apply to doc updates etc.